Skip to content

Support string enums in LiquidDoc parameter types - #1310

Open
clauderic wants to merge 4 commits into
mainfrom
liquiddoc-string-literal-unions
Open

clauderic wants to merge 4 commits into
mainfrom
liquiddoc-string-literal-unions

Conversation

@clauderic

@clauderic clauderic commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

What are you adding in this PR?

LiquidDoc @param types can now be string enums:

{% doc %}
  @param {'heading' | 'small'} [variant] - Text style
{% enddoc %}
  • Theme Check accepts these declarations. It checks literal arguments to render (including with/for aliases), content_for and block against the allowed values, and suggests each allowed value as a replacement. When adding a missing required argument to render or content_for, it inserts the first allowed value. Dynamic values such as variables are not checked. Block parameters backed by a schema setting keep the setting's type, as in Validate block call parameters #1306.
  • The language server keeps the allowed values through assignments and default, shows them in hovers and completion docs, and inserts the first one in render, content_for and block parameter completions.
  • The parser no longer ends a LiquidDoc type at a } inside a quoted string. If a quote is left open, the type still ends at the first }, as before.

Enums are modeled as unions of string literal types rather than as a dedicated enum kind. Unions such as {string | number} or {product | collection} and discriminated unions (#223) can therefore build on this without another change to the model. For now, only string literals parse as union members.

The four commits are meant to be reviewed in order: parser fix, declarations, argument checks, language server.

What's next? Any followup issues?

  • General unions such as {string | number} and {product | collection}: the parser has to accept type names as union members, and the checks and type system need rules for mixed unions.
  • Discriminated unions: Support discriminated unions #223.
  • Enums on schema-backed block parameters: the setting's type wins, so an enum declared for a text or select setting is not enforced at call sites yet.

Tophatting

In a snippet snippets/text.liquid that declares the @param above:

  1. Hover {{ variant }}: it shows 'heading' | 'small'.
  2. Hover style after {% assign style = variant | default: 'heading' %}: it still shows 'heading' | 'small'.
  3. In a template, {% render 'text', variant: 'body' %} is reported, with suggestions to replace 'body' with 'heading' or 'small'.
  • I added screenshots of the changes (before and after the changes if applicable)

Before you deploy

  • I included a minor bump changeset
  • My feature is backward compatible
  • I included a patch bump changeset

🤖 Generated with Claude Code

@clauderic
clauderic requested a review from a team as a code owner September 29, 2026 19:51
@clauderic
clauderic force-pushed the liquiddoc-string-literal-unions branch from b24a75f to c038b0f Compare September 29, 2026 20:31
@clauderic
clauderic force-pushed the liquiddoc-string-literal-unions branch from c038b0f to 78895cd Compare September 30, 2026 15:59

@charlespwd charlespwd left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Comment on lines +251 to +294
function findTypeEnd(text: string, start: number): number {
if (text[start] !== '{') return -1;

let quote: string | undefined;
let recoveryBrace = -1;

for (let pos = start + 1; pos < text.length; pos++) {
const ch = text[pos];
if (ch === '\n' || ch === '\r') break;

if (quote) {
if (ch === quote) {
if (
recoveryBrace !== -1 &&
/^[ \t]*(?:\[[^\]]*\]|[\w][\w-]*)[ \t]+/.test(text.slice(recoveryBrace + 1, pos))
) {
let next = pos + 1;
while (text[next] === ' ' || text[next] === '\t') next++;
// An unmatched quote can close at an apostrophe in the parameter's
// description, which need not have a dash. A real type delimiter wins;
// otherwise prefer a recognizable parameter boundary, even at EOL.
// This also recovers ambiguous input with a missing outer brace and a
// parameter-looking name inside its last string literal.
if (text[next] !== '|' && text[next] !== '}') {
return recoveryBrace + 1;
}
}
quote = undefined;
recoveryBrace = -1;
} else if (ch === '}' && recoveryBrace === -1) {
recoveryBrace = pos;
}
continue;
}

if (ch === '}') return pos + 1;
// Liquid strings do not interpret backslash escapes.
if (ch === "'" || ch === '"') quote = ch;
}

// Keep a malformed, closed annotation recognizable to semantic checks, and
// preserve its parameter name even when a string quote is missing.
return recoveryBrace === -1 ? -1 : recoveryBrace + 1;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuk. Can we not tokenize and consume tokens here instead? Why are we going ch by ch? This feels fast but also gross.

I feel like a stack based parser here would make this a bit easier to follow. I don’t like the lookback.

Comment on lines +248 to +256
it('keeps the schema type of a setting that LiquidDoc declares as a string enum', async () => {
const block = blockSource(
[{ id: 'variant', type: 'text' }],
["@param {'heading' | 'small'} [variant] - Variant"],
);

expect(await definitions(block)).toEqual([]);
expect(await run("{% block 'card', variant: 'body' %}{% endblock %}", block)).toEqual([]);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’re missing a failure case?

Comment on lines +139 to +145
it('preserves the existing behavior when no docset is supplied', async () => {
const source = `{% doc %}\n @param {'heading' |} variant\n{% enddoc %}`;
const offenses = await runLiquidCheck(ValidDocParamTypes, source, undefined, {
themeDocset: undefined,
});
expect(offenses).toHaveLength(0);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t that be an error?

Comment on lines +55 to +58
let start = node.paramType.position.start - 1;
let end = node.paramType.position.end + 1;
while (/[ \t]/.test(node.source.charAt(start - 1))) start--;
while (/[ \t]/.test(node.source.charAt(end))) end++;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sus?

Comment on lines +52 to +64
const compatibility = isArgumentTypeCompatible(expectedType, argument);
if (compatibility === true) return;

if (compatibility === undefined) {
// Aliases also check named Liquid types and arrays. Validate the
// declaration first so malformed types do not cause a second error.
if (!context.themeDocset) return;
validParamTypesPromise ??= context.themeDocset
.liquidDrops()
.then((entries) => new Set(getValidParamTypes(entries).keys()));
if (!parseParamType(await validParamTypesPromise, expectedType)) return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compatibility flipping on undefined is weird. What? Weird type design

kind: NodeTypes.LiquidVariable;
node: LiquidVariable;
offset: number;
resolvedType?: InferredType;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is htis here?

@charlespwd

Copy link
Copy Markdown
Contributor

packages/theme-check-common/src/checks/valid-render-snippet-argument-types/index.ts:18

Should doc be updated since this is also in blocks now?

@charlespwd

Copy link
Copy Markdown
Contributor

(my diffnotes) those are meant for my agent oops

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants